Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplication of string identifiers in Phase #724

Closed

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Oct 7, 2019

Deprecate Phase::name in favor of Phase::id

Currently, there is a duplication of Phase::name() and Phase::id(), where the former is supposed to be unique, whereas the latter is consistent with the phase id specified in the input file. In current Cantera, however, the context is always clear, so the differentiation is not needed.

A differentiation of name and phase_id does make sense for Solution objects in the Python interface (and potentially serialization of Solution in C++). To preserve this capability, the name is reassigned to the C++ object Solution (see #696).

Further:

Currently, phase name/id can be used when accessing species within a Phase, i.e.

 * A species name may be referred to via three methods:
 *
 *    -   "speciesName"
 *    -   "PhaseId:speciesName"
 *    -   "phaseName:speciesName"

The latter are, however, not used. I.e. per @speth's comment in #696

I believe that these latter two methods are not actually in use within Cantera, and I'm really not sure that we need or want them. For multiphase mixtures (C++ class MultiPhase, Python class Mixture, you always need to provide both a phase name/index as well as species name in order to look up information about a species in a particular phase. And for surfaces, the usual approach is that the species names are unique among the phases, e.g. H2O is a gas species and H2O(s) is the same species absorbed onto the surface. So at some point, I was planning to deprecate and remove this capability anyway.

Changes proposed in this pull request:

  • Replace references to Phase::name by Phase::id in equilibrium calculations and thermo phases
  • Deprecate alternate access methods for SpeciesIndex in Cantera 2.5

Pending

At this point, the only instance where Phase::name is still required is the Python interface (several tests check name rather than ID). Once #696 is merged, name will be part of the C++ Solution object, i.e. Phase::name() can be deprecated.

@ischoegl ischoegl force-pushed the deprecate-name-in-species-index branch 3 times, most recently from 7c56586 to ff15cdf Compare October 7, 2019 01:35
@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #724 into master will increase coverage by <.01%.
The diff coverage is 71.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #724      +/-   ##
==========================================
+ Coverage   70.69%   70.69%   +<.01%     
==========================================
  Files         372      372              
  Lines       43605    43606       +1     
==========================================
+ Hits        30825    30827       +2     
+ Misses      12780    12779       -1
Impacted Files Coverage Δ
include/cantera/thermo/Phase.h 100% <ø> (ø) ⬆️
src/thermo/Phase.cpp 82.52% <0%> (-0.37%) ⬇️
src/thermo/PureFluidPhase.cpp 58.7% <0%> (ø) ⬆️
src/thermo/BinarySolutionTabulatedThermo.cpp 85.24% <0%> (ø) ⬆️
src/clib/ct.cpp 8.72% <0%> (ø) ⬆️
src/equil/MultiPhaseEquil.cpp 77.72% <0%> (ø) ⬆️
src/equil/vcs_prob.cpp 94.49% <100%> (ø) ⬆️
src/equil/vcs_prep.cpp 76.92% <100%> (ø) ⬆️
src/equil/MultiPhase.cpp 75.22% <100%> (ø) ⬆️
test/thermo/phaseConstructors.cpp 100% <100%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55fe223...5bf6357. Read the comment docs.

@ischoegl ischoegl force-pushed the deprecate-name-in-species-index branch from ff15cdf to ea65ba6 Compare October 7, 2019 02:19
@ischoegl ischoegl changed the title Deprecate name/id support in Phase::speciesIndex Deprecate phase name/id support in Phase::speciesIndex Oct 7, 2019
* reference phases using Phase::id instead
* create private vcs_VolPhase::m_phaseID
* vcs_VolPhase::PhaseID() replaces vcs_VolPhase::PhaseName
* clarify conflation of 'phase id' in vcs_solve (which references a numeric
phase number) rather than a string 'phase id' as used elsewhere
* i.e. VCS_SOLVE::m_phaseNum replaces VCS_SOLVE::m_phaseID
@ischoegl ischoegl force-pushed the deprecate-name-in-species-index branch from 7daad1f to 3c98504 Compare October 7, 2019 17:01
@ischoegl ischoegl changed the title Deprecate phase name/id support in Phase::speciesIndex Remove duplication of string identifiers in Phase Oct 7, 2019
@ischoegl ischoegl force-pushed the deprecate-name-in-species-index branch from 319c204 to 5bf6357 Compare October 7, 2019 18:49
@ischoegl ischoegl closed this Oct 10, 2019
@ischoegl ischoegl deleted the deprecate-name-in-species-index branch December 16, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant